Skip to content

Hlinting partial functions #2634

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jan 30, 2022
Merged

Hlinting partial functions #2634

merged 4 commits into from
Jan 30, 2022

Conversation

mxxun
Copy link
Contributor

@mxxun mxxun commented Jan 24, 2022 •
edited by gitpod-io bot
Loading

Proof of concept for the first step of #2514: installing CI that would prevent new usages of partial functions while permitting legacy use.

My intention in this PR is to get an agreement on plausibly-controversial decisions before going full speed on implementing them.

I have taken the liberty of promoting the "Avoid restricted function" hint from a warning to an error. As I understand, novel uses of partial functions are thoroughly undesired, and merely Warning on them would not be enough to prevent appearances in code: hlint CI output was empty on installation and contains some warnings now.
(It is still possible to add exceptions, and to add warning-level hints against functions; I can document that if desired.)
(Alternatively, there might be a way to make CI hlinting action throw partial successes / warnings / other visible indication of something being wrong whenever there are warnings, which would serve to bring contributor's and reviewers' attention to them.)

Questions I am not sure I can decide myself:

  1. It's probably a good form to install a pre-commit check mirroring the CI check; should I?
  2. The current .hlint.yaml resides in ghcide/; should it be moved to the root, and applied to all subprojects? (I see a pro in the uniform enforcement of the code style, and a con in that plugin owners may want their own code style conventions.)
  3. There are some uses of partials in plugins; should these be covered by the repo-wide CI?
  4. At the moment there are many tens, possibly hundreds of files using partial functions; listing them all in .hlint.yaml might be unwieldy.
  5. Ideally, future refactorings of partial functions should be able to remember that these hlint exceptions exist, and remove them together with partials; I'm not sure how to achieve that.

jneira
jneira approved these changes Jan 25, 2022
View reviewed changes
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, I am for leverage hlint rules

@jneira
Copy link
Member

jneira commented Jan 25, 2022

It's probably a good form to install a pre-commit check mirroring the CI check; should I?

I would not interfere local development for now, the CI check at the end of the main work seems to be the preferred workflow

The current .hlint.yaml resides in ghcide/; should it be moved to the root, and applied to all subprojects? (I see a pro in the uniform enforcement of the code style, and a con in that plugin owners may want their own code style conventions.)

There are some uses of partials in plugins; should these be covered by the repo-wide CI?

At the moment there are many tens, possibly hundreds of files using partial functions; listing them all in .hlint.yaml might be unwieldy.

maybe we could start for each subpackage (in separate prs) starting with the easier ones, with a minimal hlint file in the root which passes linting with no changes

Ideally, future refactorings of partial functions should be able to remember that these hlint exceptions exist, and remove them together with partials; I'm not sure how to achieve that

not sure if it is possible automatically but a section about hlint in the contributing guide could help with that

@jneira
Copy link
Member

jneira commented Jan 25, 2022

related #2519 by @jhrcek , it was an attempt to replace the partial function Text.head and it seems it could be reopened as smaller prs. I think add the removed partial functions to hlint could be an ideal complement

@michaelpj
Copy link
Collaborator

Personally, I would prefer to bring the .hlint.yaml to the top level. That might require adding a lot of exceptions, but I think it would be worth it, personally. Either that or we fix some, as was attempted in the other PR.

@jhrcek
Copy link
Collaborator

jhrcek commented Jan 26, 2022

Sorry, I didn't get to resurrecting the PR where I started to get rid of those partial functions.
Moving hlint to root of the repo sounds like a good plan indeed.
From there we could start removing usages of partial functions one by one, in series of small non-controversial PRs.

@jneira jneira requested a review from pepeiborra as a code owner January 30, 2022 15:38
@jneira jneira added the merge me Label to trigger pull request merge label Jan 30, 2022
@mergify mergify bot merged commit 95de49b into haskell:master Jan 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jneira jneira jneira approved these changes

@pepeiborra pepeiborra Awaiting requested review from pepeiborra

Assignees
No one assigned
Labels
merge me Label to trigger pull request merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mxxun @jneira @michaelpj @jhrcek